V3.1 audit fixes #255
Open
V3.1 audit fixes #255
+1,328
−1,510
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Coverage SummaryTotals
FilesExpand
|
|
Contract comparison - from e3fed00 to 38538b9
|
bridge-proxy: refund refactor
alyn509
previously approved these changes
Dec 2, 2024
andreiblt1304
previously approved these changes
Dec 2, 2024
CostinCarabas
dismissed stale reviews from andreiblt1304 and alyn509
The base branch was changed.
bridge-proxy/src/bridge-proxy.rs
Outdated
|
|
||
| if call_data.endpoint.is_empty() | ||
| let non_empty_args = call_data.args.is_some(); | ||
| if (call_data.endpoint.is_empty() && non_empty_args) |
There was a problem hiding this comment.
what if i send empty endpoint and some arguments?
There was a problem hiding this comment.
Then the if statement would be executed and we would
self.finish_execute_gracefully(tx_id);
return;
There was a problem hiding this comment.
Wrote an integration test for this. Apparently, if I send this RAW SC call data 000000000000000002faf08000 (empty function and 50 mil gas limit) it does not refund the transfer. Good catch @dragos-rebegea
There was a problem hiding this comment.
Changed the condition to
if call_data.endpoint.is_empty()
self.finish_execute_gracefully(tx_id);
return;
Added unit test.
| "gas": "*", | ||
| "refund": "*" | ||
| } | ||
| }, | ||
| { | ||
| "step": "checkState", |
There was a problem hiding this comment.
this can be kept as it is, onyl the unwrap-token and the expect message is different, right ?
There was a problem hiding this comment.
Correct. Added the check
bridged-tokens-wrapper/scenarios/remove_wrapped_token.scen.json
Outdated
Show resolved
Hide resolved
| @@ -1090,7 +1096,7 @@ fn test_unwrap_token_create_transaction_should_work() { | |||
| state | |||
| .world | |||
| .tx() | |||
| .from(OWNER_ADDRESS) | |||
| .from(MULTISIG_ADDRESS) | |||
There was a problem hiding this comment.
how can a SC call another SC?
There was a problem hiding this comment.
Added an endpoint bridgedTokensWrapperDepositLiquidity on multisig.
There was a problem hiding this comment.
if a new endpoint was added, we should use it also in tests
There was a problem hiding this comment.
removed endpoint.
…fter-refund bridge-proxy: Clear storages after refund
bridge-proxy/src/bridge-proxy.rs
Outdated
|
|
||
| if call_data.endpoint.is_empty() | ||
| let non_empty_args = call_data.args.is_some(); | ||
| if (call_data.endpoint.is_empty() && non_empty_args) |
There was a problem hiding this comment.
Wrote an integration test for this. Apparently, if I send this RAW SC call data 000000000000000002faf08000 (empty function and 50 mil gas limit) it does not refund the transfer. Good catch @dragos-rebegea
| eth_tx.tx_nonce, | ||
| ); | ||
| // emit events only for non-SC destinations | ||
| if self.blockchain().is_smart_contract(ð_tx.to) { |
There was a problem hiding this comment.
To solve what @dragos-rebegea mentioned and fix the audit finding #9, do you think it would be better to do the following:
- emit the event only if the destination address is not a SC
- remove the
transfer_performed_sc_eventand instead emit thetransfer_performed_eventin the bridge proxy if the call to the SC succeeded
| multi_transfer_esdt_proxy, | ||
| }; | ||
|
|
||
| const MAX_BOARD_MEMBERS: usize = 40; |
There was a problem hiding this comment.
why 40? I would go to a higher value like 100 or something
There was a problem hiding this comment.
This is a fix for issue 4 from the Audit document.
We iterate over all board signatures and the call may run out of gas.
Maybe an integration test from your side could help us adjust this value.
multisig/src/setup.rs
Outdated
|
|
||
| #[only_owner] | ||
| #[payable("*")] | ||
| #[endpoint(bridgedTokensWrapperDepositLiquidity)] |
There was a problem hiding this comment.
why this? The owner of the wrapper is not the multisig contract
multisig/src/lib.rs
Outdated
|
|
||
| let action_ids_mapper = self.batch_id_to_action_id_mapping(eth_batch_id); | ||
|
|
||
| for act_id in action_ids_mapper.values() { |
There was a problem hiding this comment.
we should take max values here also
| transaction_details.refund_info.initial_nonce, | ||
| ); | ||
| } | ||
| let transaction_details = self.create_transaction_common(to, OptionalValue::None); |
There was a problem hiding this comment.
require call from bridge proxy
There was a problem hiding this comment.
Added + unit test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference PR: multiversx/mx-bridge-eth-go#389